ENH: geography constructor from geoarrow #49
Conversation
This reverts commit a01dc4b.
jorisvandenbossche
force-pushed
the
geoarrow-input
branch
from
030fb60
to
b2ff23d
Compare
| auto geog_ptr = std::make_unique<spherely::Geography>(std::move(s2geog_ptr)); | ||
| // return PyObjectGeography::from_geog(std::move(geog_ptr)); | ||
| rptr[i] = py::cast(std::move(geog_ptr)); |
There was a problem hiding this comment.
| auto geog_ptr = std::make_unique<spherely::Geography>(std::move(s2geog_ptr)); | |
| // return PyObjectGeography::from_geog(std::move(geog_ptr)); | |
| rptr[i] = py::cast(std::move(geog_ptr)); | |
| rptr[i] = make_py_geography(std::move(s2geog_ptr)); |
which needs
#include "creation.hpp"| ) | ||
|
|
||
| # if we force to not orient, we get an error | ||
| with pytest.raises(RuntimeError, match="Inconsistent loop orientations detected"): |
There was a problem hiding this comment.
Could we raise a ValueError in this case, assuming it is easy to catch the exception from s2geography and re-raise it? Otherwise not a big deal or can be done later.
There was a problem hiding this comment.
Just catch and reraise with the same error?
There was a problem hiding this comment.
try {
// call s2geography API
} catch( const std::exception &ex ) {
throw py::value_error(ex.what());
}I haven't looked where this should be inserted, though.
| def test_from_geoarrow_no_extension_type(): | ||
| arr = pa.array(["POINT (1 1)", "POINT(2 2)", "POINT(3 3)"]) | ||
|
|
||
| with pytest.raises(RuntimeError, match="Expected extension type"): |
There was a problem hiding this comment.
Same here (RuntimeError -> ValueError)?
| The maximum distance in meters that a point must be moved to | ||
| satisfy the planar edge constraint. This is only used if `planar` | ||
| is set to True. | ||
| geometry_encoding : str, default None |
There was a problem hiding this comment.
| geometry_encoding : str, default None | |
| geography_encoding : str, default None |
Not sure which is best, though.
There was a problem hiding this comment.
Yeah, given this is for geoarrow IO and in geoarrow we generally use the "geometry" term, I thought to keep that as well for the keyword ... But either way it is not ideal.
This depends on paleolimbot/s2geography#23This also introduces the question: how to we deal with functions that depend on a specific version of s2geography? Short term we can just ensure we do a upstream release and require the latest version, but longer term we might need to have some mechanism for this? -> added some version definitions that can be used in C++
Some other notes:
from_geoarrowthat accepts any Arrow-compatible array object (through the Arrow PyCapsule interface). As a result, this is not a numpy vectorized function that will take any dimension of input, it's purely a 1D -> 1D function.__arrow_c_array__, but probably should also support__arrow_c_stream__? (given that a lot of other libraries will often only implement that)s2geography::geoarrow::Readeralready supports WKT, WKB and native (nested coordinates) encoding, so those work out of the box here as well.Right now I only accept Arrow input with an extension type and let the Reader figure out the encoding, but we could also add a argument to let the user specify "WKT"/"WKB" and then accept a plain Arrow object (without geoarrow extension type)
from_wktthat is a normal vectorized function, but the problem is that pybind11'svectorizedoesn't seem to likestd::stringargument input for a vectorized arg. -> this is done in ENH: add from_wkt/to_wkt functions #50